[gateway] draft SSE implementation #243
Conversation
WalkthroughThe changes introduce Server-Sent Events (SSE) support to the Android app. This includes adding OkHttp SSE dependencies, a new Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant GatewayService
participant SSEForegroundService
participant SSEManager
participant Server
App->>GatewayService: start(context)
GatewayService->>SSEForegroundService: start(context)
SSEForegroundService->>SSEManager: connect()
SSEManager->>Server: Establish SSE connection
Server-->>SSEManager: Send event(s)
SSEManager-->>SSEForegroundService: onEvent callback
SSEForegroundService-->>GatewayService: (optional event handling)
App->>GatewayService: stop(context)
GatewayService->>SSEForegroundService: stop(context)
SSEForegroundService->>SSEManager: disconnect()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (25)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (21)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
app/src/main/java/me/capcom/smsgateway/helpers/SSEManager.kt (1)
20-23: Consider making timeout configurable and improve scope management.The hardcoded 30-second timeout might need to be configurable for different use cases. Also, consider using
SupervisorJob()for better error isolation.- private val client = OkHttpClient.Builder() - .readTimeout(30, TimeUnit.SECONDS) - .build() - private val scope = CoroutineScope(Dispatchers.IO + Job()) + private val client = OkHttpClient.Builder() + .readTimeout(30, TimeUnit.SECONDS) + .build() + private val scope = CoroutineScope(Dispatchers.IO + SupervisorJob())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/build.gradle(2 hunks)app/src/main/AndroidManifest.xml(2 hunks)app/src/main/java/me/capcom/smsgateway/helpers/SSEManager.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt(1 hunks)
🔇 Additional comments (8)
app/build.gradle (2)
60-60: LGTM! Version variable properly defined.The OkHttp version variable is well-structured and follows the existing pattern.
84-85: OkHttp 4.10.0 & SSE added – no known CVEs found
Verified via OSS Index forpkg:maven/com.squareup.okhttp3/okhttp@4.10.0: no reported vulnerabilities. Please confirm this version is compatible with your project’s Android API levels (minSdk/targetSdk).• app/build.gradle (lines 84-85):
implementation("com.squareup.okhttp3:okhttp:$okhttp_version") implementation("com.squareup.okhttp3:okhttp-sse:$okhttp_version")app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt (3)
12-12: LGTM! Import properly added.The SSEForegroundService import follows the existing import pattern.
49-49: Good placement of SSE service startup.The SSE service is properly started after other workers, which ensures dependencies are initialized first.
57-57: Excellent service shutdown ordering.The SSE service is stopped before other workers, which is the correct order to prevent connection issues during shutdown.
app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt (1)
79-93: Excellent service lifecycle management.The companion object methods properly handle Android API differences for starting foreground services and provide clean start/stop functionality.
app/src/main/java/me/capcom/smsgateway/helpers/SSEManager.kt (2)
34-82: Excellent SSE connection implementation.The connection logic properly handles authentication, event callbacks, and error scenarios. The event listener implementation covers all necessary cases.
92-105: Well-implemented reconnection strategy.The exponential backoff pattern with reasonable delay intervals provides good resilience against connection failures.
app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt (1)
24-30: Consider adding error handling and state management.The lifecycle methods lack error handling and protection against multiple start/stop calls.
Consider adding basic state management and error handling:
+private var isStarted = false + fun start(context: Context) { + if (isStarted) return + try { eventsReceiver.start() + isStarted = true + } catch (e: Exception) { + Log.e("ReceiverService", "Failed to start events receiver", e) + throw e + } } fun stop(context: Context) { + if (!isStarted) return + try { eventsReceiver.stop() + isStarted = false + } catch (e: Exception) { + Log.e("ReceiverService", "Failed to stop events receiver", e) + } }app/src/main/java/me/capcom/smsgateway/modules/receiver/events/MessagesExportRequestedEvent.kt (1)
7-17: Consider using java.time API instead of DateThe
Dateclass is legacy and has various issues. Consider usingjava.time.Instantorjava.time.LocalDateTimefor better date/time handling.-import java.util.Date +import java.time.Instant class MessagesExportRequestedEvent( - val since: Date, - val until: Date, + val since: Instant, + val until: Instant, ) : AppEvent(NAME) { data class Payload( @SerializedName("since") - val since: Date, + val since: Instant, @SerializedName("until") - val until: Date, + val until: Instant, )app/src/main/java/me/capcom/smsgateway/services/PushService.kt (1)
31-44: Consider adding debug logging for received eventsFor better observability, consider logging the routed events.
override fun onMessageReceived(message: RemoteMessage) { try { Log.d(this.javaClass.name, message.data.toString()) val event = message.data["event"]?.let { ExternalEventType.valueOf(it) } ?: ExternalEventType.MessageEnqueued val data = message.data["data"] + + Log.d(this.javaClass.name, "Routing event: $event with data: $data") eventsRouter.route( ExternalEvent( type = event, data = data, ) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/src/main/java/me/capcom/smsgateway/helpers/SSEManager.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/events/ExternalEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/events/ExternalEventType.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/EventsReceiver.kt(3 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/events/MessageEnqueuedEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/events/SettingsUpdatedEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/events/WebhooksUpdatedEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/WebhooksUpdateWorker.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/orchestrator/EventsRouter.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/orchestrator/Module.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/orchestrator/OrchestratorService.kt(3 hunks)app/src/main/java/me/capcom/smsgateway/modules/push/events/PushMessageEnqueuedEvent.kt(0 hunks)app/src/main/java/me/capcom/smsgateway/modules/push/payloads/MessagesExportRequestedPayload.kt(0 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/EventsReceiver.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/events/MessagesExportRequestedEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/services/PushService.kt(2 hunks)
💤 Files with no reviewable changes (2)
- app/src/main/java/me/capcom/smsgateway/modules/push/payloads/MessagesExportRequestedPayload.kt
- app/src/main/java/me/capcom/smsgateway/modules/push/events/PushMessageEnqueuedEvent.kt
✅ Files skipped from review due to trivial changes (6)
- app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/WebhooksUpdateWorker.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/events/WebhooksUpdatedEvent.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/events/SettingsUpdatedEvent.kt
- app/src/main/java/me/capcom/smsgateway/modules/events/ExternalEvent.kt
- app/src/main/java/me/capcom/smsgateway/modules/events/ExternalEventType.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/events/MessageEnqueuedEvent.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/me/capcom/smsgateway/helpers/SSEManager.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt
🔇 Additional comments (9)
app/src/main/java/me/capcom/smsgateway/modules/orchestrator/Module.kt (1)
8-8: LGTM!The addition of
EventsRouteras a singleton follows the correct Koin DI pattern and aligns with the new event routing architecture.app/src/main/java/me/capcom/smsgateway/modules/orchestrator/OrchestratorService.kt (2)
21-21: LGTM!The addition of
ReceiverServiceas a dependency follows the correct constructor injection pattern.
58-58: LGTM!The service stop order follows the reverse startup sequence, which is the correct lifecycle management pattern.
app/src/main/java/me/capcom/smsgateway/modules/gateway/EventsReceiver.kt (4)
9-11: LGTM!The import updates correctly reflect the new event classes that replace the old push event architecture.
14-15: LGTM!The worker imports are correctly updated to match the new worker classes.
27-28: LGTM!The transition from
PushMessageEnqueuedEventtoMessageEnqueuedEventis correctly implemented while maintaining the same functional behavior.
61-81: LGTM!The new event collectors for
WebhooksUpdatedEventandSettingsUpdatedEventfollow the established patterns:
- Consistent logging and event handling structure
- Proper settings validation before processing
- Correct worker invocation with dependency injection
app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt (1)
22-22: LGTM!Lazy initialization of
eventsReceiveris appropriate for performance optimization.app/src/main/java/me/capcom/smsgateway/modules/receiver/EventsReceiver.kt (1)
1-30: LGTM!The
EventsReceiverimplementation follows established patterns correctly:
- Proper async event handling with coroutineScope and launch
- Consistent logging structure matching other EventsReceiver classes
- Correct dependency injection using Koin
- Clean event delegation to the service layer
The inline use of
get()for context dependency is consistent with patterns used elsewhere in the codebase.
app/src/main/java/me/capcom/smsgateway/modules/orchestrator/EventsRouter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/orchestrator/EventsRouter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/orchestrator/EventsRouter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/orchestrator/OrchestratorService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/receiver/events/MessagesExportRequestedEvent.kt
Show resolved
Hide resolved
capcom6
force-pushed
the
feature/sse-events
branch
from
8c56016 to
4c326ad
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
app/src/main/java/me/capcom/smsgateway/modules/orchestrator/OrchestratorService.kt (1)
39-53: ReceiverService lifecycle partially addressed but cleanup still needed.While
receiverService.start()is now inside the try-catch block (addressing part of the previous concern), there's still no cleanup logic if the exception handling causes an early return (line 49). The ReceiverService could remain running if other foreground services fail to start.Consider adding cleanup logic for early returns:
} catch (e: Throwable) { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S && e is ForegroundServiceStartNotAllowedException ) { + receiverService.stop(context) logsSvc.insert( LogEntry.Priority.WARN, MODULE_NAME,app/src/main/java/me/capcom/smsgateway/modules/orchestrator/EventsRouter.kt (3)
15-18: Add lifecycle management for the coroutine scopeThe
EventsRoutercreates a coroutine scope but never cancels it. This could lead to memory leaks if the router instance is no longer needed but coroutines are still running.
21-45: Remove redundant nested coroutine launchesThe outer launch already runs on the IO dispatcher, so the nested launches for each event type are redundant and create unnecessary coroutines.
33-38: Handle null data gracefully instead of using requireNotNullUsing
requireNotNullwill throw anIllegalArgumentExceptionifevent.datais null, which could crash the app. Consider handling this case more gracefully.app/src/main/java/me/capcom/smsgateway/modules/receiver/events/MessagesExportRequestedEvent.kt (1)
23-26: Add error handling and validation for payload parsingThe
withPayloadmethod could throw exceptions during JSON parsing and doesn't validate that the date range is valid (since <= until).app/src/main/java/me/capcom/smsgateway/services/PushService.kt (1)
30-31: valueOf safety issue remains unaddressed.The previous concern about
ExternalEventType.valueOf(it)potentially throwingIllegalArgumentExceptionfor invalid event strings still exists.-val event = message.data["event"]?.let { ExternalEventType.valueOf(it) } - ?: ExternalEventType.MessageEnqueued +val event = message.data["event"]?.let { eventString -> + try { + ExternalEventType.valueOf(eventString) + } catch (e: IllegalArgumentException) { + Log.w(this.javaClass.name, "Unknown event type: $eventString, defaulting to MessageEnqueued") + ExternalEventType.MessageEnqueued + } +} ?: ExternalEventType.MessageEnqueued
🧹 Nitpick comments (3)
app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt (1)
24-30: Consider whether Context parameter is needed.The start() and stop() methods accept a Context parameter but don't use it. While this maintains consistency with other service interfaces in the orchestrator, consider if this parameter will be needed for future functionality or if it can be removed for clarity.
If the Context parameter isn't planned for future use, consider simplifying:
- fun start(context: Context) { + fun start() { eventsReceiver.start() } - fun stop(context: Context) { + fun stop() { eventsReceiver.stop() }Alternatively, add a comment indicating the parameter is reserved for future use.
app/src/main/java/me/capcom/smsgateway/helpers/SSEManager.kt (1)
22-24: Consider adding connection timeout to OkHttpClientWhile the read timeout is set to 1 hour for SSE, consider adding a connection timeout for initial connection establishment.
private val client = OkHttpClient.Builder() + .connectTimeout(30, TimeUnit.SECONDS) .readTimeout(1, TimeUnit.HOURS) .build()app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt (1)
122-122: Consider using STOP_FOREGROUND_REMOVE for better cleanup.Using
stopForeground(true)is deprecated. Consider using the newer API for better clarity.-stopForeground(true) +if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { + stopForeground(STOP_FOREGROUND_REMOVE) +} else { + @Suppress("DEPRECATION") + stopForeground(true) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
app/src/main/res/drawable-hdpi/notif_realtime_events.pngis excluded by!**/*.pngapp/src/main/res/drawable-mdpi/notif_realtime_events.pngis excluded by!**/*.pngapp/src/main/res/drawable-xhdpi/notif_realtime_events.pngis excluded by!**/*.pngapp/src/main/res/drawable-xxhdpi/notif_realtime_events.pngis excluded by!**/*.pngapp/src/main/res/drawable-xxxhdpi/notif_realtime_events.pngis excluded by!**/*.png
📒 Files selected for processing (25)
app/build.gradle(2 hunks)app/src/main/AndroidManifest.xml(2 hunks)app/src/main/java/me/capcom/smsgateway/helpers/SSEManager.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/helpers/SettingsHelper.kt(0 hunks)app/src/main/java/me/capcom/smsgateway/modules/events/ExternalEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/events/ExternalEventType.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/EventsReceiver.kt(3 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt(5 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/events/MessageEnqueuedEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/events/SettingsUpdatedEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/events/WebhooksUpdatedEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/WebhooksUpdateWorker.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/notifications/NotificationsService.kt(2 hunks)app/src/main/java/me/capcom/smsgateway/modules/orchestrator/EventsRouter.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/orchestrator/Module.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/orchestrator/OrchestratorService.kt(3 hunks)app/src/main/java/me/capcom/smsgateway/modules/push/events/PushMessageEnqueuedEvent.kt(0 hunks)app/src/main/java/me/capcom/smsgateway/modules/push/payloads/MessagesExportRequestedPayload.kt(0 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/EventsReceiver.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/modules/receiver/events/MessagesExportRequestedEvent.kt(1 hunks)app/src/main/java/me/capcom/smsgateway/services/PushService.kt(3 hunks)app/src/main/res/values/strings.xml(1 hunks)
💤 Files with no reviewable changes (3)
- app/src/main/java/me/capcom/smsgateway/helpers/SettingsHelper.kt
- app/src/main/java/me/capcom/smsgateway/modules/push/events/PushMessageEnqueuedEvent.kt
- app/src/main/java/me/capcom/smsgateway/modules/push/payloads/MessagesExportRequestedPayload.kt
🔇 Additional comments (31)
app/build.gradle (2)
60-60: LGTM - OkHttp version variable added.The addition of the
okhttp_versionvariable follows the existing pattern for dependency version management.
84-86: Verify OkHttp version alignment with Ktor clientThe POM inspection didn’t surface a direct OkHttp dependency for
io.ktor:ktor-client-okhttp:2.2.4. Please manually confirm which OkHttp version that artifact brings in (via its POM or BOM) and ensure your$okhttp_version(currently 4.10.0) matches it to avoid classpath conflicts.• File: app/build.gradle, lines 84–86
implementation("com.squareup.okhttp3:okhttp:$okhttp_version") implementation("com.squareup.okhttp3:okhttp-sse:$okhttp_version")app/src/main/java/me/capcom/smsgateway/modules/notifications/NotificationsService.kt (2)
95-95: LGTM - Notification ID added correctly.The new notification ID follows the existing sequential pattern and integrates well with the notification system.
27-27: Drawable Resource ExistsVerified that
notif_realtime_events.pngis present underapp/src/main/res/drawable-hdpi,drawable-mdpi,drawable-xhdpi,drawable-xxhdpi, anddrawable-xxxhdpi. No further action required.app/src/main/res/values/strings.xml (1)
122-122: LGTM - String resource added correctly.The new string resource follows the naming convention and provides clear user feedback for the SSE service notification.
app/src/main/java/me/capcom/smsgateway/modules/orchestrator/Module.kt (1)
8-8: LGTM - EventsRouter singleton registration added correctly.The EventsRouter is properly registered as a singleton following the established pattern and Koin best practices.
app/src/main/java/me/capcom/smsgateway/modules/events/ExternalEventType.kt (2)
1-1: LGTM - Package restructuring improves organization.Moving from
pushtoeventspackage better reflects the broader usage of this enum for both push and SSE events.
5-17: No remaining references to the oldEventclass found – allEvent-suffixed types are distinct classes.
The grep only surfaced usages ofDeviceRegisteredEvent,WebHookEvent,SmsEventPayload, etc., which are unrelated to the renamedExternalEventType. All imports fromme.capcom.smsgateway.modules.push.Eventhave been removed.app/src/main/java/me/capcom/smsgateway/modules/events/ExternalEvent.kt (1)
3-6: LGTM! Clean and well-designed data class.The
ExternalEventdata class provides a clear contract for external events with appropriate type safety through theExternalEventTypeenum and flexibility with the nullabledataproperty. This design supports the new SSE event architecture effectively.app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewaySettings.kt (2)
22-24: LGTM! Follows established patterns.The
fcmTokenproperty implementation is consistent with other properties in the class, using the same getter/setter pattern withKeyValueStorage. The nullable type is appropriate for FCM tokens that may not always be available.
39-39: LGTM! Consistent constant naming.The
FCM_TOKENconstant follows the established naming convention used by other storage keys in the class.app/src/main/java/me/capcom/smsgateway/modules/gateway/workers/WebhooksUpdateWorker.kt (1)
49-49: LGTM! Improved naming consistency.Changing the worker name from "CloudUpdateWorker" to "WebhooksUpdateWorker" aligns the identifier with the class name and better reflects the worker's specific purpose of handling webhook updates.
app/src/main/java/me/capcom/smsgateway/modules/gateway/events/SettingsUpdatedEvent.kt (1)
5-9: LGTM! Follows established event class pattern.The
SettingsUpdatedEventclass correctly extendsAppEventand follows the standard pattern with aNAMEconstant. The implementation is clean and appropriate for a simple event notification.app/src/main/java/me/capcom/smsgateway/modules/gateway/events/MessageEnqueuedEvent.kt (1)
5-8: LGTM! Consistent event class implementation.The
MessageEnqueuedEventclass follows the same established pattern as other event classes, extendingAppEventwith a properNAMEconstant. This supports the migration from push-based to SSE-based event architecture.app/src/main/java/me/capcom/smsgateway/modules/gateway/events/WebhooksUpdatedEvent.kt (1)
1-9: LGTM! Clean event class implementation.The event class follows a consistent pattern with the companion object containing the event name constant. This approach ensures type safety and consistency across the event system.
app/src/main/java/me/capcom/smsgateway/modules/gateway/EventsReceiver.kt (3)
9-17: LGTM! Import updates align with new event architecture.The imports correctly reflect the transition from push-based to SSE-based event handling with the new event classes and SSEForegroundService.
29-30: Good update to use the new MessageEnqueuedEvent.The change from PushMessageEnqueuedEvent to MessageEnqueuedEvent aligns with the refactoring away from push-specific event handling.
63-95: Well-structured new event collectors with appropriate conditional logic.The new collectors for WebhooksUpdatedEvent, SettingsUpdatedEvent, and DeviceRegisteredEvent follow a consistent pattern. The conditional logic for starting SSEForegroundService (lines 91-93) is particularly well thought out - only starting when gateway is enabled and FCM token is null, which makes sense for fallback SSE connectivity.
app/src/main/AndroidManifest.xml (1)
30-34: ✅ Previous security and API compliance issues have been resolved.The service declaration now correctly includes:
android:exported="false"for security (prevents external access)android:foregroundServiceType="dataSync"for API 29+ complianceThis addresses the concerns raised in previous reviews.
app/src/main/java/me/capcom/smsgateway/modules/orchestrator/OrchestratorService.kt (2)
21-21: Good addition of ReceiverService dependency.The ReceiverService integration follows the established pattern for service dependencies in the orchestrator.
57-57: Proper service shutdown order maintained.The ReceiverService is stopped first, which is appropriate for the service lifecycle management.
app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt (1)
22-22: Good use of lazy initialization for EventsReceiver.The lazy initialization ensures the EventsReceiver is only created when needed, which is efficient for lifecycle management.
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt (1)
56-56: Good improvements to lifecycle management and push token handlingThe changes improve the code by:
- Adding proper SSE service lifecycle management
- Making push token handling more robust with nullable types
- Ensuring FCM token is always stored after registration
- Simplifying the update logic
Also applies to: 111-111, 136-136, 158-174
app/src/main/java/me/capcom/smsgateway/helpers/SSEManager.kt (1)
89-97: Good implementation of disconnect with proper cleanupThe disconnect method properly handles:
- Setting the disconnecting flag to prevent reconnection attempts
- Cancelling the event source
- Cancelling all coroutine children
app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt (4)
45-47: Null safety improvement implemented correctly.The previous concern about using
requireNotNullwithout proper error handling has been addressed. The custom error message provides clear context for debugging.
72-77: Lock acquisition safety checks properly implemented.The safety checks for wake lock and WiFi lock acquisition prevent potential exceptions from acquiring already-held locks.
94-96: onBind method correctly implemented.The method now properly returns null instead of throwing a TODO exception, which is appropriate for a foreground service not designed for binding.
116-121: Lock release safety checks properly implemented.The isHeld checks before releasing locks prevent potential exceptions from releasing already-released locks.
app/src/main/java/me/capcom/smsgateway/services/PushService.kt (3)
43-49: Exception handling improvement implemented correctly.The previous concern about using
printStackTrace()has been properly addressed with structured logging using both Android Log and LogsService.
36-41: Clean refactoring to centralized event routing.The migration from inline event handling to using EventsRouter improves code organization and maintainability. The routing approach provides better separation of concerns.
62-87: Firebase token handling simplified appropriately.The token retrieval callback was streamlined while maintaining proper error logging and worker integration.
app/src/main/java/me/capcom/smsgateway/modules/gateway/services/SSEForegroundService.kt
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/receiver/EventsReceiver.kt
Show resolved
Hide resolved
capcom6
force-pushed
the
feature/sse-events
branch
from
4c326ad to
0a94d1e
Compare
🤖 Pull request artifacts
|
Summary by CodeRabbit